Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Remove check from FillStringChecked and rename it to CopyStringContent #85880

Merged
merged 10 commits into from
May 9, 2023
Merged

Remove check from FillStringChecked and rename it to CopyStringContent #85880

merged 10 commits into from
May 9, 2023

Conversation

EgorBo
Copy link
Member

@EgorBo EgorBo commented May 6, 2023 •
edited
Loading

It seems that in all uses it's garanteed that this path will never be taken. Ideally, JIT could prove it itself if it could recognize FastAllocateString(int len) and know that output has length == len (and is never null) but that is not trivial to do (the len part).

As a bonus, improves codegen for e.g.:

string Test() => Append("This repo contains", "the code to build");

string Append(string a, string b) => a + b;

(ps: the new github C#'s syntax highlighter is terrible 😢)

Current codegen for Test():

; Method Program:Test():System.String:this	
       push     rsi	
       sub      rsp, 32	
       vzeroupper 	
       mov      ecx, 35	
       call     System.String:FastAllocateString(int):System.String	
       mov      ecx, dword ptr [rax+08H]	
       cmp      ecx, 18	
       jl       SHORT G_M6843_IG04	
       lea      rdx, bword ptr [rax+0CH]	
       mov      r8, rdx	
       mov      r9, 0x19D00006880      ; 'This repo contains'	
       add      r9, 12	
       vmovdqu  ymm0, ymmword ptr [r9]	
       vmovdqu  xmm1, xmmword ptr [r9+14H]	
       vmovdqu  ymmword ptr [r8], ymm0	
       vmovdqu  xmmword ptr [r8+14H], xmm1	
       sub      ecx, 18	
       cmp      ecx, 17	
       jl       SHORT G_M6843_IG05	
       add      rdx, 36	
       mov      rcx, 0x19D00006848      ; 'the code to build'	
       add      rcx, 12	
       vmovdqu  ymm0, ymmword ptr [rcx]	
       vmovdqu  xmm1, xmmword ptr [rcx+12H]	
       vmovdqu  ymmword ptr [rdx], ymm0	
       vmovdqu  xmmword ptr [rdx+12H], xmm1	
       add      rsp, 32	
       pop      rsi	
       ret      	
G_M6843_IG04:  
       mov      rcx, 0x7FF7AB468320      ; System.IndexOutOfRangeException	
       call     CORINFO_HELP_NEWSFAST	
       mov      rsi, rax	
       mov      rcx, rsi	
       call     [System.IndexOutOfRangeException:.ctor():this]	
       mov      rcx, rsi	
       call     CORINFO_HELP_THROW	
G_M6843_IG05:  
       mov      rcx, 0x7FF7AB468320      ; System.IndexOutOfRangeException	
       call     CORINFO_HELP_NEWSFAST	
       mov      rsi, rax	
       mov      rcx, rsi	
       call     [System.IndexOutOfRangeException:.ctor():this]	
       mov      rcx, rsi	
       call     CORINFO_HELP_THROW	
       int3     	
; Total bytes of code: 190

new codegen:

; Method Program:Test():System.String:this
       sub      rsp, 40
       vzeroupper 
       mov      ecx, 35
       call     System.String:FastAllocateString(int):System.String
       cmp      byte  ptr [rax], al ;;; unfortunate nullcheck
       lea      rdx, bword ptr [rax+0CH]
       mov      rcx, 0x1E180106880      ; 'This repo contains'
       add      rcx, 12
       vmovdqu  ymm0, ymmword ptr [rcx]
       vmovdqu  xmm1, xmmword ptr [rcx+14H]
       vmovdqu  ymmword ptr [rdx], ymm0
       vmovdqu  xmmword ptr [rdx+14H], xmm1
       lea      rdx, bword ptr [rax+30H]
       mov      rcx, 0x1E180106848      ; 'the code to build'
       add      rcx, 12
       vmovdqu  ymm0, ymmword ptr [rcx]
       vmovdqu  xmm1, xmmword ptr [rcx+12H]
       vmovdqu  ymmword ptr [rdx], ymm0
       vmovdqu  xmmword ptr [rdx+12H], xmm1
       add      rsp, 40
       ret      
; Total bytes of code: 96

PS: note how everything is unrolled with simd in net8🙂

cc @stephentoub

@ghost ghost assigned EgorBo May 6, 2023
@dotnet-issue-labeler dotnet-issue-labeler bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label May 6, 2023
@EgorBo EgorBo mentioned this pull request May 7, 2023
Merged
@jkotas jkotas added area-System.Runtime and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels May 8, 2023
@ghost
Copy link

ghost commented May 8, 2023

Tagging subscribers to this area: @dotnet/area-system-runtime
See info in area-owners.md if you want to be subscribed.

Issue Details

It seems that in all uses it's garanteed that this path will never be taken. Ideally, JIT could prove it itself if it could recognize FastAllocateString(int len) and know that output has length == len (and is never null) but that is not trivial to do (the len part).

As a bonus, improves codegen for e.g.:

string Test() => Append("This repo contains", "the code to build");

string Append(string a, string b) => a + b;

(ps: the new github C#'s syntax highlighter is terrible 😢)

Current codegen for Test():

; Method Program:Test():System.String:this	
       push     rsi	
       sub      rsp, 32	
       vzeroupper 	
       mov      ecx, 35	
       call     System.String:FastAllocateString(int):System.String	
       mov      ecx, dword ptr [rax+08H]	
       cmp      ecx, 18	
       jl       SHORT G_M6843_IG04	
       lea      rdx, bword ptr [rax+0CH]	
       mov      r8, rdx	
       mov      r9, 0x19D00006880      ; 'This repo contains'	
       add      r9, 12	
       vmovdqu  ymm0, ymmword ptr [r9]	
       vmovdqu  xmm1, xmmword ptr [r9+14H]	
       vmovdqu  ymmword ptr [r8], ymm0	
       vmovdqu  xmmword ptr [r8+14H], xmm1	
       sub      ecx, 18	
       cmp      ecx, 17	
       jl       SHORT G_M6843_IG05	
       add      rdx, 36	
       mov      rcx, 0x19D00006848      ; 'the code to build'	
       add      rcx, 12	
       vmovdqu  ymm0, ymmword ptr [rcx]	
       vmovdqu  xmm1, xmmword ptr [rcx+12H]	
       vmovdqu  ymmword ptr [rdx], ymm0	
       vmovdqu  xmmword ptr [rdx+12H], xmm1	
       add      rsp, 32	
       pop      rsi	
       ret      	
G_M6843_IG04:  
       mov      rcx, 0x7FF7AB468320      ; System.IndexOutOfRangeException	
       call     CORINFO_HELP_NEWSFAST	
       mov      rsi, rax	
       mov      rcx, rsi	
       call     [System.IndexOutOfRangeException:.ctor():this]	
       mov      rcx, rsi	
       call     CORINFO_HELP_THROW	
G_M6843_IG05:  
       mov      rcx, 0x7FF7AB468320      ; System.IndexOutOfRangeException	
       call     CORINFO_HELP_NEWSFAST	
       mov      rsi, rax	
       mov      rcx, rsi	
       call     [System.IndexOutOfRangeException:.ctor():this]	
       mov      rcx, rsi	
       call     CORINFO_HELP_THROW	
       int3     	
; Total bytes of code: 190

new codegen:

; Method Program:Test():System.String:this
       sub      rsp, 40
       vzeroupper 
       mov      ecx, 35
       call     System.String:FastAllocateString(int):System.String
       cmp      byte  ptr [rax], al ;;; unfortunate nullcheck
       lea      rdx, bword ptr [rax+0CH]
       mov      rcx, 0x1E180106880      ; 'This repo contains'
       add      rcx, 12
       vmovdqu  ymm0, ymmword ptr [rcx]
       vmovdqu  xmm1, xmmword ptr [rcx+14H]
       vmovdqu  ymmword ptr [rdx], ymm0
       vmovdqu  xmmword ptr [rdx+14H], xmm1
       lea      rdx, bword ptr [rax+30H]
       mov      rcx, 0x1E180106848      ; 'the code to build'
       add      rcx, 12
       vmovdqu  ymm0, ymmword ptr [rcx]
       vmovdqu  xmm1, xmmword ptr [rcx+12H]
       vmovdqu  ymmword ptr [rdx], ymm0
       vmovdqu  xmmword ptr [rdx+12H], xmm1
       add      rsp, 40
       ret      
; Total bytes of code: 96

PS: note how everything is unrolled with simd in net8🙂

cc @stephentoub

Author: EgorBo
Assignees: EgorBo
Labels:

area-System.Runtime, needs-area-label
Milestone: -

jkotas
jkotas reviewed May 8, 2023
View reviewed changes
src/libraries/System.Private.CoreLib/src/System/String.Manipulation.cs
@@ -447,7 +466,7 @@ public static string Concat(params string?[] values)
// If it's too long, fail, or if it's empty, return an empty string.
if (totalLengthLong > int.MaxValue)
Copy link
Member

@jkotas jkotas May 8, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Is it better to do this check as if (totalLengthLong > int.MaxValue) or as if (totalLength64 != totalLength32) like what we do in other places?

It would be nice to have uniform style.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd say whatever reads better? Surpisingly, int.MaxValue shows slightly better codegen:

static void Foo1(int a, int b)
{
    long result64 = (long)a + (long)b;
    int result32 = (int)result64;
    if (result32 != result64)
        throw new OverflowException();
    Console.WriteLine(result32);
}

static void Foo2(int a, int b)
{
    long result = (long)a + (long)b;
    if (result > int.MaxValue)
        throw new OverflowException();
    Console.WriteLine((int)result);
}

https://www.diffchecker.com/JS15izhA/ (same on arm64)

@build-analysis build-analysis bot mentioned this pull request May 9, 2023
Closed
@EgorBo
Copy link
Member Author

EgorBo commented May 9, 2023

wasm failures are #85949

@EgorBo EgorBo merged commit 7d54c05 into dotnet:main May 9, 2023
@EgorBo EgorBo deleted the opt-string-fillchecked branch May 9, 2023 01:09
@ghost ghost locked as resolved and limited conversation to collaborators Jun 8, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@stephentoub stephentoub stephentoub left review comments

@jkotas jkotas jkotas approved these changes

Assignees

@EgorBo EgorBo

Labels
area-System.Runtime
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@EgorBo @stephentoub @jkotas